Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement call method for elements in CDGA's #36329

Merged
merged 12 commits into from
Mar 25, 2024
Merged

Implement call method for elements in CDGA's #36329

merged 12 commits into from
Mar 25, 2024

Conversation

miguelmarco
Copy link
Contributor

Fixes #36328 by implementing the method.

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

@miguelmarco miguelmarco requested a review from tscrim September 25, 2023 11:47
tscrim
tscrim requested changes Sep 26, 2023
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I am not 100% sure we should force the user to pass in every value, it is consistent with the multivariate polynomial ring.

I would also avoid code duplication for dict input and keyword input and just combine the two and go from there.

Having things made explicit in the doc about mixing input types and the expected behavior could be useful later on.

src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py
Comment on lines +1607 to +1608
elif values:
raise ValueError("number of arguments does not match number of variables in parent")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By this point, you have not used/parsed the keywords. So if someone passes in a mixture, then this might give a false result. In general, you probably should at least document the behavior and what you expect to have happen.

src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
miguelmarco and others added 5 commits October 24, 2023 15:03
@miguelmarco @tscrim
Update src/sage/algebras/commutative_dga.py
cdd4142 
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@miguelmarco @tscrim
Update src/sage/algebras/commutative_dga.py
396c07a 
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@miguelmarco @tscrim
Update src/sage/algebras/commutative_dga.py
bc1e77d 
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@miguelmarco @tscrim
Update src/sage/algebras/commutative_dga.py
f099194 
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@miguelmarco @tscrim
Update src/sage/algebras/commutative_dga.py
f933f0c 
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@miguelmarco
Copy link
Contributor Author

I would also avoid code duplication for dict input and keyword input and just combine the two and go from there.

This is in some sense unavoidable, since in the case of dict, the keys are elements of the algebra, whereas in the case of keywords, the keys are strings. We could try to unify them, but then we would need code for that. It won't be really better than having two five-line loops that are close to duplicated.

tscrim
tscrim requested changes Nov 2, 2023
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably would just make every dict comparison done by strings, but okay, let it be so once a few other very minor changes are done.

src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
src/sage/algebras/commutative_dga.py Outdated Show resolved Hide resolved
miguelmarco and others added 3 commits February 24, 2024 12:23
@miguelmarco @tscrim
Update src/sage/algebras/commutative_dga.py
a1d6238 
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@miguelmarco @tscrim
Update src/sage/algebras/commutative_dga.py
6ffd806 
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@miguelmarco @tscrim
Update src/sage/algebras/commutative_dga.py
24ef310 
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@tscrim
Copy link
Collaborator

tscrim commented Feb 24, 2024 •
edited
Loading

Thank you.

If you could just squash the (reviewer changes) commits, then it will be a positive review.

@miguelmarco
Copy link
Contributor Author

done!

@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit 320f177; changes) is ready! 🎉

tscrim
tscrim approved these changes Feb 28, 2024
View reviewed changes
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@vbraun vbraun merged commit b575734 into sagemath:develop Mar 25, 2024
17 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tscrim tscrim tscrim approved these changes

Assignees
No one assigned
Labels
c: algebra t: enhancement
Projects
None yet
Milestone
sage-10.4
Development

Successfully merging this pull request may close these issues.

Implement __call__method for element of CDGA's
4 participants
@miguelmarco @tscrim @vbraun @mkoeppe